fix: add centralized appendToOutput helper for null-safe output writes in after-hooks#1762
Open
uyu423 wants to merge 2 commits intocode-yeongyu:devfrom
Open
fix: add centralized appendToOutput helper for null-safe output writes in after-hooks#1762uyu423 wants to merge 2 commits intocode-yeongyu:devfrom
uyu423 wants to merge 2 commits intocode-yeongyu:devfrom
Conversation
…s in after-hooks MCP tools (Atlassian, Exa, Chrome DevTools, grep.app) can return responses where output.output is undefined. While d5fd918 fixed read operations (toLowerCase), write operations (output.output +=) were still unguarded, causing either TypeError crashes or silent 'undefinedstring' concatenation. Introduces appendToOutput() helper that: - Initializes null/undefined output to the appended text (preserving context injection instead of silently skipping) - Appends to existing string output normally - Leaves non-string output (objects/arrays) untouched Replaces inline typeof guards across 12 hook files with the centralized helper, and adds 8 unit tests for the helper. Affected hooks: - agent-usage-reminder, category-skill-reminder - claude-code-hooks/tool-execute-after-handler - comment-checker/cli-runner (2 locations) - context-window-monitor, delegate-task-retry - directory-agents-injector, directory-readme-injector - edit-error-recovery, interactive-bash-session - rules-injector, task-reminder
…ession-hook - Fix accidental 1-space indentation shift in 10 modified hook files - Guard output.output += in interactive-bash-session-hook.ts (was missed)
Contributor
|
All contributors have signed the CLA. Thank you! ✅ |
Author
|
I have read the CLA Document and I hereby sign the CLA |
Author
|
recheck |
There was a problem hiding this comment.
No issues found across 15 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Auto-approved: Refactoring PR: safely replaces direct string concatenation with a new appendToOutput helper across 13 hooks. The helper handles null/undefined MCP tool outputs (common crash source) and preserves 1
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
appendToOutput()helper to Prevent silent "undefinedstring" data corruption when MCP tools return undefined as output.output, complementing the TypeError crash fix in fix: guard output.output in tool after-hooks for MCP tools #1756 when MCP tools returnundefinedasoutput.outputoutput.output +=) across 13 hook filesChanges
Problem
#1756 fixed READ operations (
.toLowerCase()) onoutput.outputwith?? ""guards in 3 files, but WRITE operations (output.output += text) remained unguarded across 13 hook files. When MCP tools (Atlassian, Exa, Chrome DevTools, grep.app, etc.) returnundefinedinstead of a string foroutput.output, these writes cause:TypeError: undefined is not an object— crashes the entire tool execution pipeline"undefinedSome reminder text"— silent data corruption when JS coercesundefined + stringSolution
New helper (
src/hooks/hook-output-guard.ts):Design decisions:
== null(loose equality) catches bothnullandundefinedin one check13 hook files updated — all
output.output += ...andoutput.output = `${output.output}...`patterns replaced withappendToOutput()calls:agent-usage-reminder/hook.ts+= REMINDER_MESSAGE→appendToOutputcategory-skill-reminder/hook.ts+= reminderMessage→appendToOutputclaude-code-hooks/.../tool-execute-after-handler.tsappendToOutput(2 sites)comment-checker/cli-runner.ts+= result.message→appendToOutput(2 sites)context-window-monitor.ts+= CONTEXT_REMINDER→appendToOutputdelegate-task-retry/hook.ts+= guidance→appendToOutput+ READ guard?? ""directory-agents-injector/injector.ts+= directory context→appendToOutputdirectory-readme-injector/injector.ts+= README content→appendToOutputedit-error-recovery/hook.ts+= EDIT_ERROR_REMINDER→appendToOutputinteractive-bash-session/hook.ts+= reminder→appendToOutputinteractive-bash-session/interactive-bash-session-hook.ts+= reminderToAppend→appendToOutputrules-injector/injector.ts+= rule content→appendToOutputtask-reminder/hook.ts+= REMINDER_MESSAGE→appendToOutputTesting
Test cases cover:
undefined→ initializes with text (core MCP bug scenario)null→ initializes with textundefinedRelated Issues
Complements #1756 (
d5fd918) — that PR guards READ operations, this PR guards WRITE operations.Related to #1720 (original MCP tool output crash report).
Summary by cubic
Adds a centralized appendToOutput helper to make after-hook output writes null-safe, preventing crashes and "undefinedstring" corruption when MCP tools return undefined. Replaces ad-hoc string concatenation across hooks and adds tests.
Written for commit 4b21310. Summary will update on new commits.